-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correct numpy tolerence #167
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #167 +/- ##
==========================================
+ Coverage 85.87% 87.23% +1.35%
==========================================
Files 117 118 +1
Lines 12342 13710 +1368
==========================================
+ Hits 10599 11960 +1361
- Misses 1743 1750 +7 ☔ View full report in Codecov by Sentry. |
In general the PR is OK, but I'm curious if this setting has a practical effect on the calculations you've carried out? |
Nope. Code from the lastest master branch still messes up our Holstein test after I set Anyway, proposing a fix on atol is the least thing I can do at this stage : ( |
What do you mean by messing up?
…On Thu, May 9, 2024 at 6:03 AM Yu Xiong ***@***.***> wrote:
if this setting has a practical effect on the calculations you've carried
out
Nope. Code from the lastest master branch still messes up our Holstein
test after I set $RENO_NUM_THREADS to 1 and appied the new atol. I am
still investigating on why it happens on my setup.
Anyway, proposing a fix on atol is the least thing I can do at this stage
: (
—
Reply to this email directly, view it on GitHub
<#167 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIZLWMJW2PXQ7FUFOAVL7ZLZBNCXBAVCNFSM6AAAAABHONXYD2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBSGM2TENBYGQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
@jiangtong1000 I intend to open a new issue for discussion of the current problem. Please check #168 : D |
@atxy-blip ready to merge if you fix my review comment. It'll be better if you can solve the codecov issue. |
6eb51b2
to
2c37a67
Compare
Fixed. |
Thanks for the contribution! Looking forward to more of your PRs 😄 |
- originally introduced in commit 254b981
Note that this branch has been rebased and its commits are different from your local version @atxy-blip |
In commit 254b981,
atol
for numpy backend was wrongly set to1e-5
inmps/backend.py
, instead of the original value1e-8
. Meanwhile, in this version onlyatol
was set up.Renormalizer/renormalizer/mps/backend.py
Lines 136 to 140 in 254b981
As for
numpy
, the basic logic for considering two numbersa
andb
close in is:The existence of
rtol
allows for the comparison of large numbers, e.g., 10001 and 10000, which may emerge in imaginary time-evolution. Therefore, I propose adding back the rtol parameter.This commit does the following things:
mps.backend.canonical_rtol
andmps.backend.canonical_atol
mps.matrix.check_lortho
,mps.mp.check_left_canonical
,mps.mp.ensure_left_canonical
and their right counterpartsI am not sure whether
rtol
set to 1e-2 is appropriate for 32-bits. Discussions are welcome for any issue concerned : )